Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: implement basic functionality of OFREP provider #88

Merged
merged 8 commits into from
Oct 2, 2024
Merged

feat: implement basic functionality of OFREP provider #88

merged 8 commits into from
Oct 2, 2024
+360 −12

Conversation

federicobond
Copy link
Member

No description provided.

@codecov Codecov
Copy link

codecov bot commented May 7, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.45%. Comparing base (d8e10c7) to head (b15f651).
Report is 37 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   90.55%   92.45%   +1.90%     
==========================================
  Files           8       12       +4     
  Lines         180      464     +284     
==========================================
+ Hits          163      429     +266     
- Misses         17       35      +18
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@federicobond
feat: implement basic functionality of OFREP provider
a9c96f2 
Signed-off-by: Federico Bond <federicobond@gmail.com>
@federicobond federicobond force-pushed the feat/ofrep-provider-impl branch from 1ca6a60 to a9c96f2 Compare May 7, 2024 03:07
@federicobond federicobond marked this pull request as ready for review May 7, 2024 06:01
@federicobond federicobond requested a review from a team as a code owner May 7, 2024 06:01
@federicobond
refactor: reduce _resolve function complexity
535fd27 
Signed-off-by: Federico Bond <federicobond@gmail.com>
@federicobond federicobond force-pushed the feat/ofrep-provider-impl branch from 9b41a80 to 535fd27 Compare May 8, 2024 03:15
@federicobond federicobond linked an issue May 8, 2024 that may be closed by this pull request
Implement OFREP Provider #79
Closed
lukas-reining
lukas-reining reviewed May 10, 2024
View reviewed changes
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me and feels the same as for JS.
I left one question regarding validation of evaluation results.

providers/openfeature-provider-ofrep/src/openfeature/contrib/provider/ofrep/__init__.py Show resolved Hide resolved
beeme1mr
beeme1mr approved these changes May 10, 2024
View reviewed changes
mypy.ini Outdated Show resolved Hide resolved
providers/openfeature-provider-ofrep/src/openfeature/contrib/provider/ofrep/__init__.py Outdated Show resolved Hide resolved
@Kavindu-Dodan
Copy link

Good start 🙌

My main concern is this

@thomaspoignant thomaspoignant self-requested a review May 13, 2024 07:37
@gruebel
add types-requests dependency
b06c54a 
Signed-off-by: gruebel <anton.gruebel@gmail.com>
gruebel
gruebel reviewed May 13, 2024
View reviewed changes
Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job technically, just added some small suggestions

providers/openfeature-provider-ofrep/src/openfeature/contrib/provider/ofrep/__init__.py Outdated Show resolved Hide resolved
providers/openfeature-provider-ofrep/src/openfeature/contrib/provider/ofrep/__init__.py Show resolved Hide resolved
@federicobond
refactor: use NoReturn for _handle_error type
ecbdbaf 
Signed-off-by: Federico Bond <federicobond@gmail.com>
@federicobond
feat: implement type checking for resolved values
873f5e2 
Signed-off-by: Federico Bond <federicobond@gmail.com>
@federicobond
feat: take a headers_factory callable instead of fixed headers in OFR…
b7b5e27 
…EPProvider

Signed-off-by: Federico Bond <federicobond@gmail.com>
@federicobond federicobond force-pushed the feat/ofrep-provider-impl branch from b9eec89 to b7b5e27 Compare May 21, 2024 09:08
@lukas-reining
Copy link
Member

Hey @open-feature/sdk-python-maintainers, what is missing from your side, especially @federicobond to get this released?
It would be great for us to have OFREP in Python, so if I can help to get it through, I would be happy to help.

@gaby gaby mentioned this pull request Aug 22, 2024
Open
@beeme1mr
Copy link
Member

beeme1mr commented Oct 2, 2024

@lukas-reining, I think it's worth getting this out there. I'll merge when the CI passes. Thanks for kicking this off, @federicobond, and sorry for the delay.

@beeme1mr beeme1mr merged commit cb42cc0 into open-feature:main Oct 2, 2024
17 checks passed
@github-actions github-actions bot mentioned this pull request Oct 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukas-reining lukas-reining lukas-reining left review comments

@gruebel gruebel gruebel left review comments

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan left review comments

@beeme1mr beeme1mr beeme1mr approved these changes

@toddbaert toddbaert Awaiting requested review from toddbaert

@thomaspoignant thomaspoignant Awaiting requested review from thomaspoignant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement OFREP Provider
5 participants
@federicobond @Kavindu-Dodan @lukas-reining @beeme1mr @gruebel